Use bytes to represent memo text #259
Conversation
CHANGELOG.md
Outdated
| @@ -2,6 +2,10 @@ | |||
|
|
|||
| As this project is pre 1.0, breaking changes may happen for minor version bumps. A breaking change will get clearly notified in this log. | |||
|
|
|||
| ## 0.12.0 | |||
|
|
|||
| * Represent memo text contents as bytes because a memo text may not be valid UTF-8 string. | |||
There was a problem hiding this comment.
Let's link to the issue (#257) from the changelog
Co-Authored-By: Eric Saunders <ire-and-curses@users.noreply.github.com>
xdr/Stellar-transaction.x
Outdated
| @@ -335,7 +335,7 @@ union Memo switch (MemoType type) | |||
| case MEMO_NONE: | |||
| void; | |||
| case MEMO_TEXT: | |||
| string text<28>; | |||
| opaque text<28>; | |||
There was a problem hiding this comment.
Where do we keep the source of truth for our .x files? Does this get updated there also so that all other SDKs also get updated?
There was a problem hiding this comment.
I think they are kept here https://github.com/stellar/stellar-core/tree/master/src/xdr
I'm waiting for confirmation in stellar/go#2022 that we're going to change the type of the memo text field. Once we have confirmation we'll need to update the .x files in stellar core and in the go monorepo
There was a problem hiding this comment.
Files in xdr should be exactly the same as in stellar-core repo, we shouldn't change them here nor autogenerated files in: src/test/java/org/stellar/sdk/xdr.
There was a problem hiding this comment.
I'm waiting for confirmation in stellar/go#2022 that we're going to change the type of the memo text field. Once we have confirmation we'll need to update the .x files in stellar core and in the go monorepo
There was a problem hiding this comment.
If this is merged after the changes are in stellar-core master then 👍.
There was a problem hiding this comment.
I don't think we should merge it in the current form (we shouldn't change xdr files). This will be a problem when the files change in stellar-core repo (need to manually find the changes we added and apply them on top of core changes).
Maybe a solution could be adding a new file in xdr with Transaction class that parses memo correctly? This isn't ideal too but at least won't break when xdr is updated/regenerated.
xdr/Stellar-transaction.x
Outdated
| @@ -335,7 +335,7 @@ union Memo switch (MemoType type) | |||
| case MEMO_NONE: | |||
| void; | |||
| case MEMO_TEXT: | |||
| string text<28>; | |||
| opaque text<28>; | |||
There was a problem hiding this comment.
Files in xdr should be exactly the same as in stellar-core repo, we shouldn't change them here nor autogenerated files in: src/test/java/org/stellar/sdk/xdr.
|
David wasn't in favor of changing the type of the memo field, stellar/go#2022 (comment) . So I was left with no choice except for changing the implementation of how XDR strings were represented in the Java code generator: stellar/xdrgen#48 I have removed the changes to xdr/Stellar-transaction.x and regenerated the java classes using the version of xdrgen from my pull request ( stellar/xdrgen#48 ). Please review stellar/xdrgen#48 first and then review this PR |
|
|
||
| import static com.google.common.base.Preconditions.checkNotNull; | ||
|
|
||
| /** | ||
| * Represents MEMO_TEXT. | ||
| */ | ||
| public class MemoText extends Memo { | ||
| private String text; | ||
| private byte[] text; |
There was a problem hiding this comment.
If text was an XdrString then we'd keep the UTF-8 conversion knowledge in one place, in the XdrString class. Right now that knowledge is spread out across XdrString and line 19 of this class, and any other class like MemoText that follows this pattern in the future.
|
|
||
| if (new XdrString(this.name).getBytes().length > 64) { | ||
| throw new IllegalArgumentException("name cannot exceed 64 bytes"); | ||
| } |
There was a problem hiding this comment.
This change seems to be unrelated to the rest of this change. The rest of this change is specifically about memo text, but this change is to do with manage data operations. It seems out of scope and like it belongs in a separate PR.
There was a problem hiding this comment.
if I separated this change in a new PR it would depend on this branch because it requires XdrString. Lets keep both changes in the same PR for now. It shouldn't be too burdensome to review the extra changes
Fixes #257
The Java SDK assumes that all memo texts must be valid UTF 8 strings. However, that assumption is not valid. It turns out that any sequence of bytes is allowed to appear in the memo field (see stellar/go#2022 ).
Therefore, the correct representation for the memo field is a byte array.